🐛 [Bug-Fix]: add lock to avoid data race #2360 #2368
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The fix is to protect the access to s.db and save the result to a local variable.
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
yanke-xu
changed the title
yanke-xu
changed the title
|
Now that I look at it, I think Conn() was suppose to return a pointer. Like the other Storage providers do. |
ReneWerner87
approved these changes
Mar 14, 2023
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
This doesn’t make sense at all. The map is still returned as a reference and not as a copy, so this is just as racy as before.
|
|
okay, then we optimize it further @UtopiaGitHub can you provide example code where we can then get the race condition so we can compare the old and new condition/behavior |
3 tasks
|
Can we revert this change? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The fix is to protect the access to s.db and save the result to a local variable.
Description
Fixed inconsistency and also potential data race in proto/table_unmarshal.go:
u.reqFields is read/written 4 times in proto/table_unmarshal.go; 3 out of 4 times it is protected by u.lock.Lock(); 1 out of 4 times it is read without a Lock, which is in func unmarshal() on L260.
A data race may happen when unmarshal() and other func like computeUnmarshalInfo() are called in parallel.
In order to avoid potential data race here, I use u.lock.RLock(); defer u.lock.RUnlock() to make sure that all usages of items is in critical section.
Fixes #2360
Type of change
[ ] Bug fix (non-breaking change which fixes an issue)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
Checklist:
For new functionalities I follow the inspiration of the express js framework and built them similar in usage
[ ] I have performed a self-review of my own code
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation - /docs/ directory for https://docs.gofiber.io/
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] New and existing unit tests pass locally with my changes
[ ] If new dependencies exist, I have checked that they are really necessary and agreed with the maintainers/community (we want to have as few dependencies as possible)
[ ] I tried to make my code as fast as possible with as few allocations as possible
[ ] For new code I have written benchmarks so that they can be analyzed and improved